Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accommodate for two different EKOs in Evolution #289

Merged
merged 42 commits into from
Jul 18, 2024

Conversation

Radonirinaunimi
Copy link
Member

@Radonirinaunimi Radonirinaunimi commented May 30, 2024

Addresses #288. So far, this is just a bare brute-force implementation to test if it conceptually works.

Remaining to do:

  • add some unit tests
  • add checks that verify the compatibility between the initial states and the EKOs (Moved to Pineko)
  • Fix the CLI to accept different EKOs
  • deprecate Grid::evolve in favor of Grid::evolve_with_slice_iter
  • simplify the passing of EKOs
  • make sure changes from Allow to specify multiple convolution functions with CLI #293 are taken into account: before merging this branch into master
  • remove the methods Grid::evolve2 and its Python wrapper; confirm that you don't use it in Pineko

@Radonirinaunimi Radonirinaunimi added the enhancement New feature or request label May 30, 2024
@Radonirinaunimi Radonirinaunimi self-assigned this May 30, 2024
@Radonirinaunimi Radonirinaunimi marked this pull request as draft May 30, 2024 16:10
@Radonirinaunimi Radonirinaunimi linked an issue May 30, 2024 that may be closed by this pull request
pineappl/src/grid.rs Outdated Show resolved Hide resolved
@cschwan
Copy link
Contributor

cschwan commented May 31, 2024

This goes in the right direction, but I'd like to generalize this even further for the cases where we have 3 or even arbitrarily many evolutions; let's design the interface with arbitrarily many evolutions/convolutions in mind, and then let's focus first on the important cases.

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs. Then we'll need - I suppose - a single object that returns EKOs for each Convolution type. This object should replace operator_a, operator_b and slice_info.

@Radonirinaunimi
Copy link
Member Author

This goes in the right direction, but I'd like to generalize this even further for the cases where we have 3 or even arbitrarily many evolutions; let's design the interface with arbitrarily many evolutions/convolutions in mind, and then let's focus first on the important cases.

That's right! I absolutely agree with this.

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs. Then we'll need - I suppose - a single object that returns EKOs for each Convolution type. This object should replace operator_a, operator_b and slice_info.

Having a single object that replaces operator_a, operator_b, ..., operator_n would indeed be the best and cleanest solution (although I don't have a clear idea yet how that object should look like). I am currently still in the process of familiarizing myself with all the parts, but will propose something very soon xd.

@felixhekhorn
Copy link
Contributor

I wonder if this feature is a priority? since fitting with several distributions is rather a mid- or long-term feature ... in the short term we want to close one convolution (with a fixed PDF), produce a new grid with only one open convolution and turn that into an FK table, wouldn't you say so?

@Radonirinaunimi
Copy link
Member Author

I wonder if this feature is a priority? since fitting with several distributions is rather a mid- or long-term feature ... in the short term we want to close one convolution (with a fixed PDF), produce a new grid with only one open convolution and turn that into an FK table, wouldn't you say so?

That's absolutely true! But if this could be achieved with more or less the same efforts, that'll be a better no? But I agree that if this turns out to be more difficult, we could delay it for later.

@cschwan
Copy link
Contributor

cschwan commented May 31, 2024

I believe a priority should be to have a stable interface, which can be stable for a long time. This means it should account for all the various EKOs possible, and then the most general implementation can take longer as I already said in #289 (comment).

Note that for now, we don't even use the new Grid::evolve_with_slice_iter in Pineko, which is already the third iteration of evolution in PineAPPL (first was Grid::convolute_eko, second was Grid::evolve).

@Radonirinaunimi
Copy link
Member Author

Radonirinaunimi commented May 31, 2024

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs.

Thinking about this a bit, we can use Grid::convolutions to check if the grid requires different EKOs, but ultimately the different EKOs have to be passed as inputs from pineko (or from the CLI) - and this is really what should define the "EKO object".

So, I think the easiest and best solution would be to pass a list of EKOs to:

pineappl/pineappl/src/grid.rs

Lines 1400 to 1406 in 95b3bc3

pub fn evolve(
&self,
operator_a: ArrayView5<f64>,
operator_b: ArrayView5<f64>,
info: &OperatorInfo,
order_mask: &[bool],
) -> Result<FkTable, GridError> {

in that we replace operator_ with list_operators, and then check with Grid::convolutions that the number and types of EKOs are consistent with the initial states.

In this way, we can arbitrarily pass different evolutions. Does this sound reasonable?

@felixhekhorn
Copy link
Contributor

@cschwan original idea was based on an iterative approach: calling evolve several times with one operator each and closing one (multi-)index at a time - I think this might be conceptually easier then dealing with all convolutions at the same time. Ofc there can always be some sugar to do all in one go

@cschwan
Copy link
Contributor

cschwan commented May 31, 2024

@cschwan original idea was based on an iterative approach: calling evolve several times with one operator each and closing one (multi-)index at a time - I think this might be conceptually easier then dealing with all convolutions at the same time. Ofc there can always be some sugar to do all in one go

Indeed, but using this approach it's easy to get it wrong, so I think I've changed my mind 😄.

@cschwan
Copy link
Contributor

cschwan commented May 31, 2024

In this way, we can arbitrarily pass different evolutions. Does this sound reasonable?

That's what I had in mind, although I'm still unclear about the details.

@giacomomagni
Copy link

Hi all,
since now we made quite a few progress in NNPDF/pineko#181 and we are ready to support
pineappl==0.7.4 in Pineko, would it be possible to merge this PR and have a new micro release, so these changes
are more accessible.
In particular we will need to introduce FkTable.convolve_with_two also in n3fit.
Thanks!!

@Radonirinaunimi
Copy link
Member Author

Radonirinaunimi commented Jun 4, 2024

@cschwan, @felixhekhorn: After having looked a bit, I think generalizing to any arbitrary evolution is complicated and requires a lot of changes (because for example, most of the structs will have to be modified).

As far as this PR is concerned, this already achieves the initial intended purposes, and we already tested it in NNPDF/pineko#181. In this sense, at least, I think this can be reviewed. What do you think?

@Radonirinaunimi Radonirinaunimi marked this pull request as ready for review June 4, 2024 23:25
@cschwan
Copy link
Contributor

cschwan commented Jul 12, 2024

@Radonirinaunimi I've implemented most of the necessary changes, in particular the Python binding for Grid::evolve_with_slice_iter2. In pineappl_py/tests/test_fk_table.py you can see an example how to use this function. Could you try to adapt Pineko to use this function?

@giacomomagni
Copy link

@Radonirinaunimi I've implemented most of the necessary changes, in particular the Python binding for Grid::evolve_with_slice_iter2. In pineappl_py/tests/test_fk_table.py you can see an example how to use this function. Could you try to adapt Pineko to use this function?

This seems to work NNPDF/pineko@d62dac4
thanks @cschwan

@Radonirinaunimi
Copy link
Member Author

I finally had time to carefully look into this again. Thanks again the for changes @cschwan (and especially for the improvements), these look really good to me. I believe we've achieved what we ought to do in this PR, should this be merged now?

@cschwan
Copy link
Contributor

cschwan commented Jul 18, 2024

We didn't manage to fully generalize the evolution interfaces, but that wasn't the intention of this pull request so that's OK. In the future, we'll have to change it again, but so is life.

@Radonirinaunimi we can almost merge, can you please take remove the methods Grid::evolve2 and its Python wrapper; confirm that you don't use it in Pineko.

@Radonirinaunimi
Copy link
Member Author

We didn't manage to fully generalize the evolution interfaces, but that wasn't the intention of this pull request so that's OK. In the future, we'll have to change it again, but so is life.

@Radonirinaunimi we can almost merge, can you please take care of the following:

* [ ]  remove the methods `Grid::evolve2` and its Python wrapper; confirm that you don't use it in Pineko

Yes, I can do this.

@cschwan
Copy link
Contributor

cschwan commented Jul 18, 2024

When you've removed the methods, you can merge this branch into master. Then I'm going to prepare a new release 0.8.1 that will have the new method.

@Radonirinaunimi
Copy link
Member Author

When you've removed the methods, you can merge this branch into master. Then I'm going to prepare a new release 0.8.1 that will have the new method.

Perfect! Awesome.

The method has been removed, I will wait for the tests to succeed and then merge. Once v0.8.1 is available I will propagate it to nnpdf and pineko to make the CI working again.

Thanks again 🚀

@cschwan
Copy link
Contributor

cschwan commented Jul 18, 2024

Great, thank you!

@Radonirinaunimi Radonirinaunimi merged commit e17d122 into master Jul 18, 2024
9 checks passed
@Radonirinaunimi Radonirinaunimi deleted the extend_eko_convolution branch July 18, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the evolution to accommodate for two different EKOs
4 participants